Skip to content

feat(connectors): browser.evaluate connector + owletto submodule bump#828

Merged
buremba merged 6 commits into
mainfrom
feat/owletto-chrome-executor-bump
May 18, 2026
Merged

feat(connectors): browser.evaluate connector + owletto submodule bump#828
buremba merged 6 commits into
mainfrom
feat/owletto-chrome-executor-bump

Conversation

@buremba
Copy link
Copy Markdown
Member

@buremba buremba commented May 17, 2026

Summary

Pairs with owletto-web PR #153 (chrome.debugger executor inside Owletto for Chrome). Adds the server-side connector definition that makes the new extension executor invocable from the admin UI + watcher dispatch, and bumps the owletto submodule pointer so the matching extension code lands.

What's in this PR

  • packages/connectors/src/browser_evaluate.ts (new) — generic "run JS in a user's signed-in Chrome via chrome.debugger" connector. requiredCapability: 'browser.debugger', runtime.platforms: ['chrome-extension']. configSchema mirrors the executor's payload contract:
    • url (optional) — navigate first
    • script (required) — JS expression for Runtime.evaluate(awaitPromise: true)
    • wait_for_selector + wait_timeout_ms — polled selector gate before eval
    • open_in_new_tabdefaults true for the same trust-boundary reason as the extension side: a compromised gateway / leaked worker token shouldn't be able to drive the tab the user is actively using
    • close_tab_after — defaults to open_in_new_tab
  • packages/connectors/src/index.ts — export the new connector. The catalog auto-discovers packages/connectors/src/*.ts so registration is automatic, but the index export is needed for direct imports from the connector worker.
  • packages/owletto — bump submodule pointer to feat/chrome-debugger-executor HEAD so the extension's runBrowserEvaluate lands with the connector definition.

Why this is small

The architecture was already there (device_worker_id pinning, approved_inputaction_input plumbing, runtime.platforms: ['chrome-extension'] auto-wiring) — only the missing pieces were the extension-side executor (in PR #153) and this one server-side definition file. No new gateway code, no new dispatch logic, no new run protocol — the existing chrome.tabs connector is the same shape.

End-to-end verification

Done locally with a fake mini-gateway + Playwright-loaded Chrome + the Owletto extension built from the submodule pointer in this PR. The executor opened a background tab, attached chrome.debugger, navigated to example.com, evaluated ({ title: document.title, h1: document.querySelector('h1')?.textContent }), streamed back {"title":"Example Domain","h1":"Example Domain"}, posted complete, closed the tab — all in ~2 seconds with no leftover yellow debugger banner. Full log in PR #153's e2e comment.

Test plan

Follow-ups

  • Typed connector helpers in the SDK — connector authors today craft raw JS strings. A navigate(url) + snapshot() + click(ref) + evaluate(fn) DSL on the gateway side would be the natural ergonomics layer. Out of scope; shaped by the first real bridge connector (Revolut feed is the most likely first consumer).
  • Signed/allowlisted scripts — the real fix to the trust boundary. The current posture (default new tab, gateway-mints-script, OAuth gates the gateway) is sufficient for v0.2 but worth tightening before opening the bridge to user-tooled scripts.
  • browser.snapshot / browser.page_text / browser.fill_form — specialized connectors that build on top of browser.evaluate. Each is a thin wrapper.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added three new Chrome extension connectors: browser.evaluate for executing scripts, browser.fill_form for automating form submissions, and browser.page_text for extracting page content.
  • Improvements

    • Enhanced connector discovery to support connectors organized in subdirectories.
  • Tests

    • Updated test formatting and consistency.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR introduces three new Chrome-extension-only browser connectors (evaluate, fill_form, page_text) with feed and event schemas, updates discovery infrastructure to support subdirectory organization and dual filename conventions, and re-exports the new connectors from the main package entry point.

Changes

Browser Connectors and Discovery Infrastructure

Layer / File(s) Summary
Browser connector definitions and runtime stubs
packages/connectors/src/browser/evaluate.ts, packages/connectors/src/browser/fill_form.ts, packages/connectors/src/browser/page_text.ts
Defines three new ConnectorRuntime subclasses with definition objects specifying feed config schemas (url, script, fields, selectors, timeouts), event metadata schemas, and cloud-side sync()/execute() methods that throw BRIDGE_ONLY to delegate work to the Chrome extension worker.
Connector discovery and resolution for subdirectories
packages/server/src/utils/connector-catalog.ts, packages/connector-worker/src/compile-connector.ts
Server-side catalog scan now collects .ts files from root and one-level subdirectories, stores catalog-relative source_path to preserve location, and worker-side findBundledConnectorFile generates both subdirectory and underscore filename candidates to resolve bundled connectors.
Package index re-exports
packages/connectors/src/index.ts
Adds wildcard re-exports of the three new browser connector modules to the main connectors entry point under a "browser primitives" section.

Sequence Diagram

The following diagram illustrates the connector discovery and worker resolution workflow:

sequenceDiagram
  participant CatalogServer as Catalog Server
  participant StorageLayer as Storage/Metadata
  participant ConnectorWorker as Connector Worker
  CatalogServer->>StorageLayer: Scan root + 1-level subdirs for .ts files
  StorageLayer-->>CatalogServer: Return candidates (excluding .d.ts)
  CatalogServer->>CatalogServer: Extract metadata, store catalog-relative source_path
  StorageLayer->>StorageLayer: Store source_path with subdirectory preserved
  ConnectorWorker->>ConnectorWorker: Generate two candidate filenames (subdir + underscore)
  ConnectorWorker->>StorageLayer: Check candidates across WORKER_CONNECTOR_DIR_CANDIDATES
  StorageLayer-->>ConnectorWorker: Return first existing match
  ConnectorWorker->>ConnectorWorker: Load bundled connector
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • lobu-ai/lobu#772: Both PRs modify the findBundledConnectorFile logic in packages/connector-worker/src/compile-connector.ts to handle connector source path resolution.

Suggested labels

skip-size-check


🐰 Three browser friends now chat with chrome so fine,
Evaluate, fill, and read—each shares the design.
The subdirectory dance, two names in the game,
Cloud stubs point homeward; the extension claims fame! 🌳

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: adding a browser.evaluate connector and bumping the owletto submodule.
Description check ✅ Passed The description is comprehensive, covering the summary, what's included, architecture rationale, E2E verification, test plan, and follow-ups as per template requirements.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/owletto-chrome-executor-bump

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 80dac2547c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +45 to +52
evaluate: {
key: 'evaluate',
name: 'Evaluate JS',
description:
'Executes a JS expression in the page and emits one event with the JSON-serialised return value.',
configSchema: {
type: 'object',
required: ['script'],
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Mark the evaluate feed user-managed

Because this device connector declares runtime + requiredCapability, getBundledDeviceConnectors() includes every feed that is not userManaged, and ensureDeviceConnectorWired() auto-creates those feeds with config = NULL and next_run_at = NOW(). For a Chrome extension advertising browser.debugger, this evaluate feed will therefore be scheduled immediately without the required script, causing recurring failed browser-evaluate syncs (default every 6 hours) until someone manually edits or pauses the auto-wired feed. Mark this feed userManaged: true (as local.directory does for required per-feed config) or don't expose it as an auto-wired feed.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/connectors/src/browser_evaluate.ts`:
- Line 108: The method declaration async sync(_ctx: SyncContext):
Promise<SyncResult> uses an underscore-prefixed unused parameter; remove the
parameter to satisfy lint rules by changing the signature to async sync():
Promise<SyncResult>. Update any references inside the function (if present) that
use _ctx (they should be removed) and ensure the function still returns a valid
SyncResult; keep the function name sync and types SyncContext/SyncResult intact
elsewhere.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7ec44843-a3e3-4988-8991-66f089b8cadf

📥 Commits

Reviewing files that changed from the base of the PR and between 3e82989 and 80dac25.

📒 Files selected for processing (3)
  • packages/connectors/src/browser_evaluate.ts
  • packages/connectors/src/index.ts
  • packages/owletto

},
};

async sync(_ctx: SyncContext): Promise<SyncResult> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Drop the unused sync parameter instead of underscore-prefixing it.

Use sync(): Promise<SyncResult> here to match repository lint/style rules for unused params.

Suggested change
-  async sync(_ctx: SyncContext): Promise<SyncResult> {
+  async sync(): Promise<SyncResult> {
     throw new Error(BRIDGE_ONLY);
   }

As per coding guidelines, “When fixing unused-parameter errors, delete the parameter rather than prefixing with _”.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async sync(_ctx: SyncContext): Promise<SyncResult> {
async sync(): Promise<SyncResult> {
throw new Error(BRIDGE_ONLY);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/connectors/src/browser_evaluate.ts` at line 108, The method
declaration async sync(_ctx: SyncContext): Promise<SyncResult> uses an
underscore-prefixed unused parameter; remove the parameter to satisfy lint rules
by changing the signature to async sync(): Promise<SyncResult>. Update any
references inside the function (if present) that use _ctx (they should be
removed) and ensure the function still returns a valid SyncResult; keep the
function name sync and types SyncContext/SyncResult intact elsewhere.

buremba added a commit that referenced this pull request May 17, 2026
Per review on PR #828: browser.evaluate (and the new browser.page_text +
browser.fill_form wrappers) aren't third-party data-source connectors —
they're primitives the Owletto for Chrome extension executes against
arbitrary pages. Grouping them under a `browser/` subdirectory makes that
structural distinction visible in the directory listing and gives a clean
home for future primitives (.screenshot, .click, .keyboard).

Layout change:
  packages/connectors/src/browser_evaluate.ts   → browser/evaluate.ts
  packages/connectors/src/browser_fill_form.ts  → browser/fill_form.ts
  packages/connectors/src/browser_page_text.ts  → browser/page_text.ts

The connector keys (browser.evaluate, browser.fill_form, browser.page_text)
do NOT change. The catalog loader + worker resolver are updated to find
files via either convention:

- packages/server/src/utils/connector-catalog.ts — listCatalogConnectorDefinitions
  now scans one level deep so `browser/*.ts` is discovered alongside
  top-level files. source_path uses relative() so subdir is preserved
  (avoids basename collisions if e.g. evaluate.ts ever existed at top
  level too).
- packages/connector-worker/src/compile-connector.ts — findBundledConnectorFile
  tries `<key.split('.').join('/')>.ts` first, then falls back to the
  flat `<key.replace(/\./g,'_')>.ts` convention. Existing flat-named
  connectors (chrome.tabs → chrome_tabs.ts, etc.) keep working unchanged.

Plus 2 new browser primitives layered on top of browser.evaluate (still
require extension-side executor branches to ship in a follow-up):
  - browser.page_text — bake a "return cleaned page text" script
  - browser.fill_form — bake a "fill these selector→value pairs" script

make typecheck + make build-packages clean.
@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/server/src/utils/connector-catalog.ts (1)

134-148: ⚠️ Potential issue | 🔴 Critical | ⚖️ Poor tradeoff

Gateway's findBundledConnectorFile only supports flat convention while worker supports both.

The worker's findBundledConnectorFile (in compile-connector.ts) supports both subdirectory (browser/evaluate.ts) and flat (chrome_tabs.ts) conventions, but the gateway's version here only supports the flat convention (line 137: ${key.replace(/\./g, '_')}.ts). This breaks resolution for actively-used subdirectory-based connectors like browser.evaluate, which exists at packages/connectors/src/browser/evaluate.ts and is used across multiple code paths (device reconciliation, worker API polling, queue helpers, and connector compilation).

Update the function to match the worker's dual-convention logic:

🔧 Proposed fix to support both conventions
 export function findBundledConnectorFile(key: string): string | null {
   const cached = bundledFileCache.get(key);
   if (cached !== undefined) return cached;
-  const fileName = `${key.replace(/\./g, '_')}.ts`;
+  const candidates = [
+    `${key.replace(/\./g, '/')}.ts`,
+    `${key.replace(/\./g, '_')}.ts`,
+  ];
   let found: string | null = null;
   for (const candidate of DEFAULT_CONNECTOR_DIR_CANDIDATES) {
-    const filePath = resolve(candidate, fileName);
-    if (existsSync(filePath)) {
-      found = filePath;
-      break;
+    for (const fileName of candidates) {
+      const filePath = resolve(candidate, fileName);
+      if (existsSync(filePath)) {
+        found = filePath;
+        break;
+      }
     }
+    if (found) break;
   }
   bundledFileCache.set(key, found);
   return found;
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/server/src/utils/connector-catalog.ts` around lines 134 - 148, The
current findBundledConnectorFile only checks the flat filename variant (dots →
underscores) and misses subdirectory-based connectors; update
findBundledConnectorFile to try both conventions when scanning
DEFAULT_CONNECTOR_DIR_CANDIDATES: for each candidate directory, check a path
with key.replace(/\./g, '_') + '.ts' and also check a path with
key.replace(/\./g, '/') + '.ts' (or otherwise preserving subdirectory layout),
returning the first match and caching via bundledFileCache as before; keep the
same function name and use the existing DEFAULT_CONNECTOR_DIR_CANDIDATES and
bundledFileCache so resolution matches the worker's dual-convention logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/connector-worker/src/compile-connector.ts`:
- Line 75: The current path check in compile-connector.ts uses a hardcoded "/"
which breaks on Windows; update the validation so it builds a platform-correct
prefix and compares using that (e.g., create a prefix from dir with path.sep or
use path.join(dir, path.sep) or use path.relative(dir, filePath) and ensure the
relative path does not start with "..") and then use that normalized comparison
instead of filePath.startsWith(`${dir}/`) to correctly detect files under dir
across OSes; adjust the logic surrounding the filePath and dir variables
accordingly.

In `@packages/connectors/src/browser/fill_form.ts`:
- Around line 97-99: The function async sync currently declares an unused
parameter `_ctx`; remove the parameter from the signature (change async
sync(_ctx: SyncContext): Promise<SyncResult> to async sync():
Promise<SyncResult>) and ensure no references to `_ctx` remain in the function
body; keep the existing throw new Error(BRIDGE_ONLY) behavior and update any
local type annotations or interface implementations if necessary to match the
new no-argument signature.

In `@packages/connectors/src/browser/page_text.ts`:
- Around line 98-100: The sync method declared as async sync(_ctx: SyncContext):
Promise<SyncResult> is using an unused parameter; remove the unused parameter so
the signature becomes async sync(): Promise<SyncResult>, update the
implementation to match (keep throwing new Error(BRIDGE_ONLY)), and ensure any
internal references/overrides or implementations expecting sync(_ctx) are
adjusted to the new no-arg signature (search for sync in the same class or
interface to update typings if necessary).

In `@packages/owletto`:
- Line 1: The CI is failing because the packages/owletto git submodule is pinned
to a commit not reachable from owletto/main; update the submodule pointer so it
references a commit that exists on owletto/main (or first merge the desired
commit into owletto/main and then repin). Locate the submodule entry for
packages/owletto (the submodule SHA stored in the repo index/.gitmodules) and
change the pinned commit to a commit reachable from owletto/main (or land the
missing commit to owletto/main and update the pin), then commit the updated
submodule reference so $PINNED will be reachable from owletto/main and CI will
pass.

---

Outside diff comments:
In `@packages/server/src/utils/connector-catalog.ts`:
- Around line 134-148: The current findBundledConnectorFile only checks the flat
filename variant (dots → underscores) and misses subdirectory-based connectors;
update findBundledConnectorFile to try both conventions when scanning
DEFAULT_CONNECTOR_DIR_CANDIDATES: for each candidate directory, check a path
with key.replace(/\./g, '_') + '.ts' and also check a path with
key.replace(/\./g, '/') + '.ts' (or otherwise preserving subdirectory layout),
returning the first match and caching via bundledFileCache as before; keep the
same function name and use the existing DEFAULT_CONNECTOR_DIR_CANDIDATES and
bundledFileCache so resolution matches the worker's dual-convention logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 48daa7c1-95a1-48b2-8cc1-8216bea347b1

📥 Commits

Reviewing files that changed from the base of the PR and between 80dac25 and df302e8.

📒 Files selected for processing (7)
  • packages/connector-worker/src/compile-connector.ts
  • packages/connectors/src/browser/evaluate.ts
  • packages/connectors/src/browser/fill_form.ts
  • packages/connectors/src/browser/page_text.ts
  • packages/connectors/src/index.ts
  • packages/owletto
  • packages/server/src/utils/connector-catalog.ts

// Belt-and-braces: the resolved path must stay under the candidate
// dir. CONNECTOR_KEY_RE already forbids `..`, but the regex doesn't
// know about our path-joining choices.
if (!filePath.startsWith(`${dir}/`)) continue;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Path validation breaks on Windows due to hardcoded forward slash.

The check uses a hardcoded / in the template literal, but resolve() returns platform-specific separators. On Windows, dir is C:\app\connectors and filePath is C:\app\connectors\browser\evaluate.ts, so the check becomes !filePath.startsWith("C:\app\connectors/") (mixed separators) which always fails.

🔧 Proposed fix using path.sep for cross-platform compatibility
-      if (!filePath.startsWith(`${dir}/`)) continue;
+      if (!filePath.startsWith(dir + path.sep)) continue;

Alternatively, use path.join() for clarity:

+import { join, resolve, sep } from 'node:path';
...
-      if (!filePath.startsWith(`${dir}/`)) continue;
+      if (!filePath.startsWith(join(dir, ''))) continue;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!filePath.startsWith(`${dir}/`)) continue;
if (!filePath.startsWith(dir + path.sep)) continue;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/connector-worker/src/compile-connector.ts` at line 75, The current
path check in compile-connector.ts uses a hardcoded "/" which breaks on Windows;
update the validation so it builds a platform-correct prefix and compares using
that (e.g., create a prefix from dir with path.sep or use path.join(dir,
path.sep) or use path.relative(dir, filePath) and ensure the relative path does
not start with "..") and then use that normalized comparison instead of
filePath.startsWith(`${dir}/`) to correctly detect files under dir across OSes;
adjust the logic surrounding the filePath and dir variables accordingly.

Comment on lines +97 to +99
async sync(_ctx: SyncContext): Promise<SyncResult> {
throw new Error(BRIDGE_ONLY);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove the unused sync parameter instead of underscore-prefixing it.

_ctx is unused; drop it to comply with repo rules.

Proposed fix
-  async sync(_ctx: SyncContext): Promise<SyncResult> {
+  async sync(): Promise<SyncResult> {
     throw new Error(BRIDGE_ONLY);
   }

As per coding guidelines, “When fixing unused-parameter errors, delete the parameter rather than prefixing with _”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/connectors/src/browser/fill_form.ts` around lines 97 - 99, The
function async sync currently declares an unused parameter `_ctx`; remove the
parameter from the signature (change async sync(_ctx: SyncContext):
Promise<SyncResult> to async sync(): Promise<SyncResult>) and ensure no
references to `_ctx` remain in the function body; keep the existing throw new
Error(BRIDGE_ONLY) behavior and update any local type annotations or interface
implementations if necessary to match the new no-argument signature.

Comment on lines +98 to +100
async sync(_ctx: SyncContext): Promise<SyncResult> {
throw new Error(BRIDGE_ONLY);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Drop the unused _ctx from sync().

The parameter is unused and should be removed per repository rule.

Proposed fix
-  async sync(_ctx: SyncContext): Promise<SyncResult> {
+  async sync(): Promise<SyncResult> {
     throw new Error(BRIDGE_ONLY);
   }

As per coding guidelines, “When fixing unused-parameter errors, delete the parameter rather than prefixing with _”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/connectors/src/browser/page_text.ts` around lines 98 - 100, The sync
method declared as async sync(_ctx: SyncContext): Promise<SyncResult> is using
an unused parameter; remove the unused parameter so the signature becomes async
sync(): Promise<SyncResult>, update the implementation to match (keep throwing
new Error(BRIDGE_ONLY)), and ensure any internal references/overrides or
implementations expecting sync(_ctx) are adjusted to the new no-arg signature
(search for sync in the same class or interface to update typings if necessary).

Comment thread packages/owletto Outdated
@buremba
Copy link
Copy Markdown
Member Author

buremba commented May 17, 2026

Status snapshot before merge

This PR has grown from the original "browser.evaluate definition + submodule bump" into the full first-cut bridge primitive set. Summary for reviewers + post-merge follow-ups so nothing falls on the floor.

What's in this PR now

  • packages/connectors/src/browser/evaluate.ts — the browser.evaluate primitive (gateway-side definition; executor is in the submodule).
  • packages/connectors/src/browser/page_text.tsbrowser.page_text wrapper (extracts cleaned text from a page; pre-baked script composed on top of browser.evaluate at the extension).
  • packages/connectors/src/browser/fill_form.tsbrowser.fill_form wrapper ({selector: value} map → input/change events, optional submit click).
  • Subdirectory move for the new browser primitives (packages/connectors/src/browser/*.ts) instead of top-level — keeps them visually distinct from the third-party-service connectors (linkedin, revolut, github, etc.). Connector keys (browser.evaluate, etc.) are unchanged.
  • Catalog + worker resolver updates to handle one-level-deep subdirectories. Existing flat-named connectors (chrome.tabschrome_tabs.ts) keep working unchanged via a fallback.
  • packages/owletto submodule bumped to 0387641 (post fix(proxy): swap placeholder secrets in auth headers unconditionally #153 merge on owletto-web/main). Submodule SHA is reachable from main.
  • Merged in lobu/main twice during the session so we don't diverge.

Submodule side (owletto-web #153) — already MERGED

  • apps/chrome/executor.js (runBrowserEvaluate) + apps/chrome/wrappers.js (script-template wrappers).
  • background.js dispatcher routes browser.evaluate, browser.page_text, browser.fill_form to the executor.
  • 17/17 unit tests pass (executor + wrappers).
  • E2E proven in a real Chrome via Playwright + fake gateway harness (see PR fix(proxy): swap placeholder secrets in auth headers unconditionally #153 comment).

Tested

  • Local: make build-packages clean, make typecheck (strict) clean.
  • CI: drift check now passes after the submodule re-bump (was failing because the parent was pinning at the pre-merge feature SHA on owletto-web).
  • E2E with the user's real Chrome + Mac app native-messaging bridge: bridge subprocess + mint-child-token cycle verified working (3 successful mints captured in gateway log). Full polling-to-stream pipeline blocked on the chrome.permissions.request user-gesture quirk — solvable but pulled out of this PR for scope. Plan posted below.

Post-merge follow-ups (each its own PR / issue)

Scope Notes
Revolut feed connector Multi-day First real bridge consumer; will pressure-test the wrappers + shape any DSL helpers.
Typed connector helpers on the gateway Half day navigate(url) / snapshot() / click(ref) / evaluate(fn) on top of run.config. Don't design until the Revolut connector is in flight.
Signed-script trust hardening Multi-day Real fix for config.script trust boundary. Default-new-tab + gateway-only minting is the v0.2 posture; tighten before opening the bridge to user-tooled scripts.
chrome.permissions.request user-gesture flow in pairing.js ~2 hrs The native-messaging auto-pair currently falls back to the URL form because permissions.request outside a user click silently fails. Either move it to a button handler or rely on static host_permissions. Demo workaround was to edit the user's manifest.json — that's not a fix that ships.
Owletto for Chrome Web Store publish Logistics Once v0.2.0 is shipped to the Web Store, the Mac app's External-Extensions-JSON drop + native-messaging allowed_origins get a stable extension ID and the silent-install + auto-pair UX works end-to-end without dev shims.
Drop / replace chrome.scripting cap in manifest ~30 min Manifest still advertises scripting cap without an executor. Either ship a browser.scripting-backed primitive or trim the cap to match what we actually handle.
browser.screenshot, browser.click Per-connector Natural next wrappers to compose. Each is ~50 LOC server def + ~30 LOC executor branch (script-template pattern is now established).

Ready to merge once CI lands.

buremba added 2 commits May 18, 2026 00:32
…ayout

Adds three Chrome-extension connectors (browser.evaluate, browser.fill_form,
browser.page_text) whose executors live in the Owletto for Chrome extension.
Definitions sit under `packages/connectors/src/browser/` so primitive
groupings are structurally distinct from third-party service connectors.

- packages/connectors/src/browser/{evaluate,fill_form,page_text}.ts (new)
- packages/connectors/src/index.ts: re-export browser/*
- packages/connector-worker/src/compile-connector.ts: resolve dotted keys
  via subdir (`browser/evaluate.ts`) in addition to the existing
  underscore-flat convention (`chrome_tabs.ts`).
- packages/server/src/utils/connector-catalog.ts: scan one level deep so
  browser/* is discovered; preserve relative `source_path` so resolvers
  don't collide on basename.

Submodule (packages/owletto) is left at main's pin (aeb3324) — the
browser.evaluate executor already shipped via #825/#159, so no bump is
needed by this PR.
Pre-existing format drift introduced in #829 was failing main's
format-lint CI for several commits. Bundling the auto-fix here so PR
#828's format-lint check turns green on top of an already-red main.
@buremba buremba force-pushed the feat/owletto-chrome-executor-bump branch from 4df94c8 to 279f66d Compare May 17, 2026 23:37
…itives

Pi review found four follow-ups to the browser/* subdir layout introduced
in the previous commit:

- packages/server/src/utils/connector-catalog.ts: server-side
  findBundledConnectorFile() was still flat-only — auto-install /
  device-reconcile / worker-poll would treat browser.evaluate as "no
  bundled source." Now mirrors the worker-side resolver (subdir first,
  underscore-flat fallback). Adds bundledConnectorSourcePath(filePath)
  so subdir paths round-trip through connectorSourcePathToUri.
- packages/server/src/utils/ensure-connector-installed.ts +
  packages/server/src/worker-api/device-reconcile.ts: stop persisting
  basename(filePath) as source_path — collides on basename across
  subdirs and breaks source_uri resolution for subdir connectors.
- packages/cli/src/commands/_lib/connector-loader.ts: CLI resolver was
  also flat-only.
- packages/connectors/src/browser/{evaluate,fill_form,page_text}.ts:
  mark feeds userManaged so device-reconcile doesn't auto-wire them
  with config=NULL. These are bridge-composing primitives (script /
  url + fields / url are required, gateway-author-supplied), not
  end-user feeds.
@buremba
Copy link
Copy Markdown
Member Author

buremba commented May 17, 2026

Force-rebased onto main + pi review applied

Rebase: dropped the merge-of-main commit + intermediate submodule bumps. The branch is now main + 3 clean commits:

  1. feat(connectors): browser.evaluate / fill_form / page_text + subdir layout — the original PR content (browser/* subdir).
  2. chore(format): biome format pre-existing diff.test.ts drift — unblock format-lint (drift introduced by feat(apply): watcher admin-only fields + lobu export + examples roll-out #829 was failing main's CI for multiple commits).
  3. fix(connectors): subdir-aware resolvers + userManaged on browser primitives — applies pi -p review findings:
    • Server-side findBundledConnectorFile() in connector-catalog.ts was still flat-only; mirrors the worker-side resolver now.
    • Added bundledConnectorSourcePath(filePath) helper so subdir paths round-trip through connectorSourcePathToUri().
    • ensure-connector-installed.ts + worker-api/device-reconcile.ts stop persisting basename(filePath) as source_path (would collide on basename / break source_uri resolution for browser/evaluate.ts).
    • CLI's connector-loader.ts resolver was also flat-only; same fix.
    • browser/{evaluate,fill_form,page_text}.ts marked userManaged: true — these have required gateway-author-supplied config (script, url + fields, url), so device-reconcile would otherwise auto-wire them with config = NULL and produce a runs-but-fails loop.

Submodule: left at aeb3324 (current main pin) — no extra bump needed, since browser.evaluate executor already shipped via #825/#159.

Known red checks inherited from main

These are pre-existing on origin/main HEAD; this PR doesn't introduce them. Confirmed by reproducing each on a clean main checkout:

  • CI / integrationpublic-pages-contract.test.ts (2 cases) + mcp/auth.test.ts (2 cases). Public-page fallback hits the discovery JSON; auth tests expect invalid_token and get unauthorized. Likely fallout from feat(auth): cookie pivot — drop bootstrap-pat.txt, add /api/auth/local-init #830 cookie pivot. Needs a separate PR.
  • PR Validation / build-testpackages/owletto/src/main.tsx:26 has a top-level await against a Vite build target that doesn't support it. Bug lives in the owletto submodule. Needs a separate PR on lobu-ai/owletto to set build.target: 'esnext' (or refactor to an async IIFE).

Green now

typecheck, unit, frontend, format-lint (newly green after this PR's chore commit), security, CodeQL, submodule-drift, mac-build-smoke, migrations.

buremba added 2 commits May 18, 2026 01:02
…w-up

- packages/server/src/workspace/multi-tenant.ts: when a Bearer header is
  present but PAT verify, OAuth verify, AND session-cookie lookup all
  fail, return RFC 6750 `invalid_token` 401 (with WWW-Authenticate
  error=invalid_token) instead of falling through to anonymous and
  returning generic `unauthorized` later. Fixes mcp/auth.test.ts
  "should reject expired/invalid OAuth access token" — they assert the
  standards-compliant error code so MCP clients (Claude Desktop etc.)
  surface "bad token" rather than mistaking it for "no auth needed."
- packages/server/src/worker-api/device-reconcile.ts: short-circuit
  ensureDeviceConnectorWired() when declaredFeedKeys is empty (every
  feed userManaged → nothing to auto-wire). Avoids a compile + upsert +
  no-auth-connection adopt per Chrome poll for browser.* primitives.
  Definition + version row still get installed lazily by
  ensureConnectorInstalled when a composing connector runs them.
  (Second-round pi review finding.)
- packages/owletto: bump pointer to 2552ed0 — fix(build): vite
  target=esnext for top-level await in main.tsx (lobu-ai/owletto#161,
  merged). Unblocks PR Validation / build-test which had been failing
  on packages/owletto's vite build since the auth-pivot landed.
The packages/web → packages/owletto rename in #817 updated
APP_ROOT-relative candidates but missed two:

- The `../web/{dist,index.html}` sibling-deploy candidate was kept
  verbatim ("for out-of-monorepo deployments"), but after the rename
  the sibling dir is `../owletto/`, not `../web/`. The stale path
  silently misses on every lookup.
- The `path.resolve(process.cwd(), '../packages/owletto/...')`
  candidate has always resolved to `packages/packages/owletto/...`
  (double `packages`) when cwd is `packages/server` — i.e. exactly
  the layout the integration job runs under. Rewriting as
  `../owletto/...` lands in the right sibling.

Symptoms: every `public-pages-contract.test.ts` (and
`mcp/auth.test.ts` indirectly) failure on `main` since 7a72456 —
`buildPublicPageModel` returned a real model but
`loadAnySpaHtmlTemplate()` returned null, so the catch-all fell
through to the JSON discovery response with `Cache-Control: no-store`.
Reproduces against PGlite locally; fixed and verified all 3
public-pages tests pass.

Same broken pattern repaired in `utils/public-origin.ts:hasLocalFrontend`
(8 candidates, 4 corrected).
@buremba
Copy link
Copy Markdown
Member Author

buremba commented May 18, 2026

All previously-red checks are now green

Final state — 5 commits, all CI checks pass except cosmetic `pr-size`:

Check Before After
typecheck, unit, frontend, security, codeQL, smoke, migrations, drift
format-lint ❌ (main drift)
build-test ❌ (owletto top-level await)
integration ❌ (public-pages + mcp-auth)
pr-size ❌ (947 lines from `diff.test.ts` auto-format — cosmetic, needs `--admin` to bypass)

What landed in the bundle

  1. `feat(connectors): browser.evaluate / fill_form / page_text + subdir layout` — the original PR content (browser/* subdir).
  2. `chore(format): biome format pre-existing diff.test.ts drift` — unblock `format-lint` (drift on main since feat(apply): watcher admin-only fields + lobu export + examples roll-out #829).
  3. `fix(connectors): subdir-aware resolvers + userManaged on browser primitives` — first pi review pass:
    • Server-side `findBundledConnectorFile()` was still flat-only (mirror of worker-side).
    • `bundledConnectorSourcePath()` helper so subdir paths round-trip through `connectorSourcePathToUri()`.
    • `ensure-connector-installed.ts` + `worker-api/device-reconcile.ts` stop persisting `basename(filePath)` as `source_path`.
    • CLI's `connector-loader.ts` resolver — same flat-only bug.
    • browser/* feeds marked `userManaged: true` (gateway-author-supplied config, never auto-wired).
  4. `fix(mcp,device-reconcile,owletto): inherited main failures + pi follow-up` — three bundled:
    • `workspace/multi-tenant.ts`: return RFC 6750 `invalid_token` 401 when Bearer header was supplied but PAT+OAuth+session lookup all failed (fixes `mcp/auth.test.ts` × 2).
    • `worker-api/device-reconcile.ts`: short-circuit `ensureDeviceConnectorWired()` when `declaredFeedKeys` is empty — second-round pi finding.
    • Submodule bump to `2552ed0` — owletto-side fix `vite target=esnext` for top-level await (lobu-ai/owletto#161, merged). Fixes `build-test`.
  5. `fix(server): repair sibling-walk SPA template/dist paths post-rename` — the `packages/web → packages/owletto` rename in refactor: rename submodule mount packages/web to packages/owletto (#789) #817 left stale `../web/...` and `../packages/owletto/...` (resolves to `packages/packages/owletto/`) candidates that silently missed. `buildPublicPageModel` returned a real model but template was null, fallthrough hit the JSON discovery handler. Reproduced + verified against PGlite locally: 3 public-pages tests + 36 mcp/auth tests pass.

Pi review

Two passes: first found 4 follow-ups (all applied in #3). Second pass found one residual auto-wire ping per Chrome poll (applied in #4). Third pass on final state pending.

Third pi review flagged two issues with the prior round:

- worker-api/device-reconcile.ts: the earlier early-return-when-empty
  short-circuited too aggressively. `local.directory` and other
  device connectors whose only feed is `userManaged` rely on
  ensureDeviceConnectorWired to install the connector_definition +
  version + connection — `/api/workers/me/feeds` 404s on
  "no connection wired" otherwise. Move the short-circuit into the
  fast-path check instead: when the connection + version already
  exist AND there's nothing to verify (declaredFeedKeys empty),
  fast-path returns without compiling. First poll still does the
  full install; subsequent polls are zero work.
- utils/connector-catalog.ts: the one-level subdir scan was
  descending into `connectors/src/__tests__/` and trying to extract
  catalog metadata from test files that import `bun:test`,
  producing esbuild warnings on every cold scan. Skip `__tests__`
  and any leading-underscore dir.
@buremba
Copy link
Copy Markdown
Member Author

buremba commented May 18, 2026

Pi review pass-3 follow-ups (commit 9363776)

Third pi review caught two regressions in the previous device-reconcile short-circuit:

  1. worker-api/device-reconcile.ts — early-return-when-feedKeys=empty was too aggressive. local.directory (and any device connector whose only feed is userManaged) needs ensureDeviceConnectorWired to install the connector_definition + version + connection so the Mac app's POST /api/workers/me/feeds can attach folder feeds — without the auto-wired connection, that endpoint 404s with "No connection wired yet". Moved the short-circuit into the fast-path check instead: first poll still installs the definition + connection; subsequent polls fast-path out.
  2. utils/connector-catalog.ts — the one-level subdir scan was descending into packages/connectors/src/__tests__/ and trying to extract catalog metadata from test files that import bun:test, producing "Could not resolve bun:test" warnings on every cold scan. Skip __tests__ + any leading-underscore dir.

Verified locally: 43 / 43 tests pass (3 public-pages + 36 mcp-auth + 4 connector-catalog).
EOF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants